Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP: Better WSDM UX #605

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

WIP: Better WSDM UX #605

wants to merge 1 commit into from

Conversation

blackspherefollower
Copy link
Collaborator

This PR is tracking the headache of having to update the user-config before a new websocket device can connect.

Right now, this simplistic change lets WS devices declare BTLE identifiers, but there's the potential that this could open things too much. As soon as a webpage can connect a device, it could spam the user-config file with new devices, even if they're not enabled by default (disabling new WS devices on connect is not implemented in this PR yet).

There's a trade-off between UX and security here, and whilst I tend to pick security I'm not sure that's the right choice here.

The prior conversation on Discord:

BlackSphereFollower — 03/01/2024 12:16
@qdot Did the wsdm break? I'm seeing it reject devices because it can't match them to a protocol, but it's not matching any because it's not comparing websocket identifiers against bluetooth identifiers. Or do all websocket protocols have to be explicilty declared?

qdot — 03/01/2024 17:56
@BlackSphereFollower I haven't touched that part of code in forever. It may just be a bug, I never really throughly tested the identifer comparison.
websocket protocols shouldn't need specific declaration, it should just be able to suss things out from the protocol/identifier passed to it.

BlackSphereFollower — 03/01/2024 18:02
Ok, I might need to PR a fix there then. I was testing out the glitch hosted speedometer and couldn't get it to work at all
https://github.com/buttplugio/buttplug/compare/dev...blackspherefollower:buttplug-rs:dev Actually before I do, does this look right?

qdot — 03/01/2024 18:28
I think so.

qdot — 03/01/2024 18:29
Oh, um, ok, thinking back on this, devices did need to be explicitly set up with a protocol in the user config. They couldn't just declare it as part of the handshake. 
That was due to security, otherwise anyone could just pop up on the websocket connector.

BlackSphereFollower — 03/01/2024 18:33
That makes total sense and I had wondered if that was the case.

qdot — 03/01/2024 18:33
That said, that's... not the worst idea as long as we gate it.

BlackSphereFollower — 03/01/2024 18:33
Disabled by default? Could still let something spam the config
A nice toast popup for "let this in or block it" might work too, but the block list could also be spammed...
I don't think there's an easy answer here

qdot — 03/01/2024 18:38
Well feel free to draft PR it or make an issue right now just to keep the idea around, but yeah the more we go over this the more the current restrction sonds like it was warrented.

@blackspherefollower
Copy link
Collaborator Author

I think I've changed my mind in regards to the potential DoS risk: the likelihood of such an attack occurring is ridiculously low and could be simply mitigated by just disabling the WSDM. I do think that connecting devices should NOT automatically be enabled, so if Intiface were to get the ability to modify the blocklist on-the-fly, I think that this would be a very nice QoL tweak for anyone dealing with WSDs

@qdot
Copy link
Member

qdot commented May 7, 2024

So the new device config work allows us to basically share a mutable device config manager between the UI and whatever buttplug server is running. The native code running behind intiface's flutter UI acts as shared memory even when dart has different isolates happening (as required for app backgrounding). Real time updates should be possible now, so yeah, I'm willing to rethink this.

@qdot qdot force-pushed the dev branch 2 times, most recently from ca203ea to 110b302 Compare October 7, 2024 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@blackspherefollower @qdot